Don't skip Handle_Increment if Do_Encrypt_NONPLAINTEXT succeeds #280
Conversation
I'm 100% sure that this was a (unfortunately disastrous) typo.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #280 +/- ##
==========================================
+ Coverage 83.29% 83.50% +0.21%
==========================================
Files 39 39
Lines 11049 11054 +5
Branches 832 832
==========================================
+ Hits 9203 9231 +28
+ Misses 1544 1518 -26
- Partials 302 305 +3 ☔ View full report in Codecov by Sentry. |
|
I think this probably also deserves a unit test to make sure this kind of thing doesn't happen again. I think that should probably go in |
|
I believe you are correct with the assumption of needing a test to catch this. If you would like to add this to AEAD_AES_GCM_BITMASK_1, within test/unit/ut_tm_apply.c, that would be great. |
|
Thank you for adding the test. |
| @@ -529,7 +529,7 @@ int32_t Crypto_TM_Do_Encrypt(uint8_t sa_service_type, SecurityAssociation_t* sa_ | |||
| } | |||
|
|
|||
|
|
|||
| if (status != CRYPTO_LIB_SUCCESS) | |||
| if (status == CRYPTO_LIB_SUCCESS) | |||
| { | |||
| status = Crypto_TM_Do_Encrypt_Handle_Increment(sa_service_type, sa_ptr); | |||
| } | |||
There was a problem hiding this comment.
Good catch. This is correct.
However, the following code. Should it perhaps be in an if (status == CRYPTO_LIB_SUCCESS) block as well?
We need to make sure to check the return of status before moving on each time. There are two additional places where this seems to occur here.
Currently, Crypto_TM_Do_Encrypt_Handle_Increment only gets called if Crypto_TM_Do_Encrypt_NONPLAINTEXT (/ _AEAD_Logic) fails which leads to the IV and ARSN being stuck at the same value if everything goes right.